[Runtime field editor] Error handling #109233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 19, 2021
sebelga
force-pushed
the
runtime-field-editor/error-handling-2
branch
from
d1e41df
to
9ef2290
Compare
sebelga
force-pushed
the
runtime-field-editor/error-handling-2
branch
from
03bf7b3
to
9b9f0f6
Compare
flash1293
reviewed
Sep 20, 2021
sebelga
force-pushed
the
runtime-field-editor/error-handling-2
branch
from
179efa3
to
c87e712
Compare
sebelga
commented
Sep 21, 2021
| @@ -217,14 +211,6 @@ const FieldEditorComponent = ({ field, onChange, onFormModifiedChange, syntaxErr | |||
| }); | |||
| }, [updatedName, updatedType, updatedScript, isValueVisible, updatedFormat, updatePreviewParams]); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
The preview panel is now always visible.
|
Another suggestion: Should we allow saving a new or modified runtime field even of the user doesn't have the privs required for the Preview? It worked in 7.14. Maybe this is the change that is small enough to make it into 7.15.1 and unblock me and other users in my same blocked state. It could be it's own PR. |
sebelga
force-pushed
the
runtime-field-editor/error-handling-2
branch
2 times, most recently
from
1e336b5
to
82da806
Compare
sebelga
force-pushed
the
runtime-field-editor/error-handling-2
branch
3 times, most recently
from
8ba1ffd
to
cfaa779
Compare
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/lens/public/datatable_visualization/components.DatatableComponent it invokes executeTriggerActions with correct context on click on top valueStandard OutStack TraceKibana Pipeline / general / X-Pack Saved Object Tagging Functional Tests.x-pack/test/saved_object_tagging/functional/tests/dashboard_integration·ts.saved objects tagging - functional tests dashboard integration editing allows to select tags for an existing dashboardStandard OutStack TraceMetrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
|
@elasticmachine merge upstream |
1 similar comment
|
@elasticmachine merge upstream |
I've updated the kbn-monaco package to expose a `validation$` Observable for the consumer. This allows the consumer to get notified when the editor is validating and if it is valid. Previously we [relied on a `setTimeout`](https://github.com/elastic/kibana/blob/master/src/plugins/index_pattern_field_editor/public/components/field_editor/form_fields/script_field.tsx#L90) to wait for the Painless validation to finish and I found it was not predictable and could easily break if for some reason the timeout changed.
In elastic#109238 I've added the possibility to provide dynamic data to field validators. When working on this current PR I realised that I didn't take into account the fact that we need to be able to validate the field without first triggering a change on the field. My initial proposal to expose an Observable (https://github.com/elastic/kibana/pull/109238/files#diff-ef2d4424a31a8d8def4c1c2d9d756cfef9cc58b30837c1b6639e3419e572cdd9R35) did not take that into account. I reverted that change and decided to allow a Promise to be provided and let the consumer in control of resolving the promise for the validation to resolve.
A nice feature of the form lib is that it lets you provide interchangeably synchronous or asynchronous validations, and it "just work". This simplicity comes at a cost though. In order to achieve that the lib first tries to execute all the validations synchronously and if it finds an asynchronous validation it re-run all the validations asynchronously. This has an undesired effect: async HTTP requests are called twice. :/ With the new `isAsync` option (which should _always_ set to `true` for async validations for the problem stated) we can explicitly tell the form lib that there are some asynchronous validations and that will prevent running them twice.
In some rare cases (and this PR required it) we need to be immediately aware of a field value change _before_ the validations are run. For that purpose I've added an optional `onChange` option that can be passed to the `useFormData()` hook.
While doing loads of QA testing with error messages I realised that the `form.getErrors()` handler was returning stale error messages and was not in sync with the fields errors.
I've updated the Painless syntax validation to use the `validations()` Observable. I've added a new validator to validate the script calling the server endpoint. I could then remove the script validation that we had in place _after_ the form was submitted.
|
Thanks for the review @yuliacech and @dborodyansky !
I haven't been able to reproduce it locally. Are you referring to keep the pin position after closing the editor and reopening?
Indeed I can reproduce it. It is strange as I would expect the same issue in other places where the Monaco editor is being used (like in Discover) but I can't reproduce it there. I opened #117873 to see if we can improve this. |
|
@debadair could you do a quick copy review of the error states on this PR? Here are the screenshots
Thanks! |
|
@elasticmachine merge upstream |
|
@sebelga Here is a screencast of pinning bug I am encountering. Pinned items are expected to stick to the top of the list and not scroll away, or hide when filtering is applied.
|
|
Thanks @dborodyansky. I don't think this PR brought that issue and the behaviour is already in master. I would prefer to address it in a separate PR if you think this should be improved. I looked at Discover and the list of pinned fields is scrolled out of the visible area as we do here
There isn't much space left to scroll if we decided to always keep them visible on top. Specially because there is no limit as to how many fields can be pinned.
|
…r/error-handling-2
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
|
@debadair I merged the PR to unblock others. I'll create a separate PR for any copy feedback you have. Cheers 👍 |
kibanamachine
added
the
backport missing
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
3 similar comments
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
sebelga
added
backport:skip
KOTungseth
added
the
Feature:Data Views
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the error handling for runtime fields in the Data view field editor.
Script errorbadgetypecorrectly._sourcewe will preview its value.Notes on implementation
kbn-monaco changes
I've updated the kbn-monaco package to expose a
validation$Observable for the consumer. This allows the consumer to get notified when the editor is validating and if it is valid. Previously we relied on asetTimeoutto wait for the Painless validation to finish and I found it was not predictable and could easily break if for some reason the timeout changed. (01f73e3)Form lib changes
Allow Promise provider instead of Observable for dynamic data: In [Form lib] Allow dynamic data to be passed to validator functions #109238 I've added the possibility to provide dynamic data to field validators. When working on this current PR I realised that I didn't take into account the fact that we need to be able to validate the field without first triggering a change on the field. My initial proposal to expose an Observable (https://github.com/elastic/kibana/pull/109238/files#diff-ef2d4424a31a8d8def4c1c2d9d756cfef9cc58b30837c1b6639e3419e572cdd9R35) did not take that into account. I reverted that change and decided to allow a Promise to be provided for dynamic data and leave the consumer in control of resolving that promise . (d0b48bf)
New
isAsyncoption for validations: A nice feature of the form lib is that it lets you provide interchangeably synchronous or asynchronous validations, and it "just work". This simplicity comes at a cost though. In order to achieve that the lib first tries to execute all the validations synchronously and if it finds an asynchronous validation it re-run all the validations asynchronously. This has an undesired effect: async HTTP requests are called twice. :/ With the newisAsyncoption (which should always set totruefor async validations for the problem stated) we can explicitly tell the form lib that there are some asynchronous validations and that will prevent running them twice. (68b77fe)New
onChangeoption to theuseFormData()hook: In some rare cases (and this PR required it) we need to be immediately aware of a field value change before the validations are run. For that purpose I've added an optionalonChangeoption that can be passed to theuseFormData()hook. (272a78d)form.getErrors()handler was returning stale error message and was not in sync with the fields errors. (36bbad8)Handled edge cases where there are no documents in the cluster
The Painless script validation (through the
_executeAPI) requires a document to be provided. Currently we don't support custom JSON to be sent by the user so we rely on retrieving sample documents from the cluster. This created a problem if there aren't any documents in the cluster or if fetching sample docs failed. In that case the validation would never resolve and the user wouldn't be able to save his runtime field. I added a flag to handle this scenario and bypass the script validation if there aren't any cluster data.Note to reviewers
There are multiple asynchronous event that makes testing this PR challenging (but interesting! 😊). I hope to have solved all the possible race condition that those timeouts created.
How to test
emit("hello"script (missing closing parentesis)--> The script should be invalid, no HTTP request. There should be a "Script error" badge
in the preview. Clicking the "Save" button won't allow saving this field
long. There script should not be valid, same behaviour as before. In the footer of the editor the reason should be displayed with an additional text: "Verify that you have correctly set the runtime field type." as this is a casting issue.--> There should be an error fetching the document. The script should still be invalid and you should not be able to save the runtime field.
keyword.--> The script should now be valid, the error fetching doc should still be there but you should be able to save the runtime field.
Test the scenario where there are no documents in the cluster
The easiest is to update the
field_preview_context.tsfile. UnderfetchSampleDocuments()change the following:Now do the same with throttling set to "Slow 3G" 😊 And play around different combinations of fetching doc, trying to save while validating the painless syntax or the painless script...